Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ls: no-trunc opt #2138

Merged
merged 1 commit into from
Oct 3, 2024
Merged

ls: no-trunc opt #2138

merged 1 commit into from
Oct 3, 2024
+322 −12

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 23, 2023 •
edited
Loading

follow-up #830 with the no-trunc opt.

When using ls, platforms can get wieldy easily and make output in the terminal difficult to read:

image

NAME/NODE            DRIVER/ENDPOINT                       STATUS     BUILDKIT               PLATFORMS
bk12                 docker-container
 \_ bk120             \_ unix:///var/run/docker.sock       running    v0.12.5                linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
bk13                 docker-container
 \_ bk130             \_ unix:///var/run/docker.sock       running    v0.13.2                linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
bk14                 docker-container
 \_ bk140             \_ unix:///var/run/docker.sock       inactive
bridge               docker-container
 \_ bridge0           \_ unix:///var/run/docker.sock       inactive
builder              docker-container
 \_ builder0          \_ unix:///var/run/docker.sock       running    v0.16.0                linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
dev                  remote
 \_ dev0              \_ docker-container://buildkit-dev   inactive
sifive               docker-container
 \_ sifive0           \_ tcp://192.168.0.65:2375           error                             linux/riscv64*
test                 docker-container
 \_ test0             \_ unix:///var/run/docker.sock       inactive
test2                docker-container
 \_ test20            \_ unix:///var/run/docker.sock       inactive
test4                docker-container
 \_ test40            \_ unix:///var/run/docker.sock       stopped
default*             docker
 \_ default           \_ default                           running    v0.15.2                linux/amd64, linux/amd64/v2, linux/amd64/v3, linux/arm64, linux/riscv64, linux/ppc64le, linux/s390x, linux/386, linux/mips64le, linux/mips64, linux/arm/v7, linux/arm/v6
docker-test          docker
 \_ docker-test       \_ docker-test                       running    v0.11.6                linux/amd64, linux/386
docker20             docker
 \_ docker20          \_ docker20                          running    v0.8+unknown           linux/amd64, linux/386
docker23             docker
 \_ docker23          \_ docker23                          running    v0.10.6+d52b2d5        linux/amd64, linux/386
docker24             docker
 \_ docker24          \_ docker24                          running    v0.11.6                linux/amd64, linux/386
remote-ssh           docker
 \_ remote-ssh        \_ remote-ssh                        running    v0.11.6+0a15675913b7   linux/arm64, linux/arm/v7, linux/arm/v6

With this change only platforms are truncated by default and only when using the table format. We only display "common" platforms with a limitation to 4 and put add a count for number of variants found:

image

NAME/NODE            DRIVER/ENDPOINT                       STATUS     BUILDKIT               PLATFORMS
bk12                 docker-container
 \_ bk120             \_ unix:///var/run/docker.sock       running    v0.12.5                linux/amd64 (+3), linux/arm64, linux/arm (+2), linux/ppc64le, (5 more)
bk13                 docker-container
 \_ bk130             \_ unix:///var/run/docker.sock       running    v0.13.2                linux/amd64 (+3), linux/arm64, linux/arm (+2), linux/ppc64le, (5 more)  
bk14                 docker-container
 \_ bk140             \_ unix:///var/run/docker.sock       inactive
bridge               docker-container
 \_ bridge0           \_ unix:///var/run/docker.sock       inactive
builder              docker-container
 \_ builder0          \_ unix:///var/run/docker.sock       running    v0.16.0                linux/amd64 (+3), linux/arm64, linux/arm (+2), linux/ppc64le, (5 more)  
dev                  remote
 \_ dev0              \_ docker-container://buildkit-dev   inactive
sifive               docker-container
 \_ sifive0           \_ tcp://192.168.0.65:2375           error                             linux/riscv64*
test                 docker-container
 \_ test0             \_ unix:///var/run/docker.sock       inactive
test2                docker-container
 \_ test20            \_ unix:///var/run/docker.sock       inactive
test4                docker-container
 \_ test40            \_ unix:///var/run/docker.sock       stopped
default*             docker
 \_ default           \_ default                           running    v0.15.2                linux/amd64 (+3), linux/arm64, linux/arm (+2), linux/ppc64le, (5 more)  
docker-test          docker
 \_ docker-test       \_ docker-test                       running    v0.11.6                linux/amd64, linux/386
docker20             docker
 \_ docker20          \_ docker20                          running    v0.8+unknown           linux/amd64, linux/386
docker23             docker
 \_ docker23          \_ docker23                          running    v0.10.6+d52b2d5        linux/amd64, linux/386
docker24             docker
 \_ docker24          \_ docker24                          running    v0.11.6                linux/amd64, linux/386
remote-ssh           docker
 \_ remote-ssh        \_ remote-ssh                        running    v0.11.6+0a15675913b7   linux/arm64, linux/arm (+2)

crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls.go Outdated
Comment on lines 295 to 366
majorPlatforms := map[string]struct{}{
"linux/amd64": {},
"linux/arm64": {},
"linux/arm/v7": {},
"linux/mips64": {},
"linux/ppc64le": {},
"linux/riscv64": {},
"linux/s390x": {},
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following platforms take priority but let me know if we should change that.

@crazy-max crazy-max force-pushed the ls-notrunc branch 2 times, most recently from db1dd93 to 94cdae5 Compare November 23, 2023 14:18
crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls_test.go Outdated
Comment on lines 50 to 104
name: "all preferred",
platforms: []string{"darwin/arm64*", "linux/arm64*", "linux/arm/v5*", "linux/arm/v6*", "linux/arm/v7*", "windows/arm64*"},
expected: []string{"linux/arm64*", "linux/arm/v7*", "darwin/arm64*", "linux/arm/v5*"},
truncated: true,
max: 4,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we always display all preferred platforms even if it exceeds the limit?

crazy-max
crazy-max commented Nov 23, 2023
View reviewed changes
commands/ls_test.go Outdated
Comment on lines 57 to 126
name: "no major preferred",
platforms: []string{"linux/amd64/v2*", "linux/arm/v6*", "linux/mips64le*", "linux/amd64", "linux/amd64/v3", "linux/386", "linux/arm64", "linux/riscv64", "linux/ppc64le", "linux/s390x", "linux/mips64", "linux/arm/v7"},
expected: []string{"linux/amd64", "linux/arm64", "linux/riscv64", "linux/ppc64le"},
truncated: true,
max: 4,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should preferred platforms take priority even if not part of major ones?

@crazy-max crazy-max mentioned this pull request Nov 23, 2023
Closed
@crazy-max crazy-max marked this pull request as ready for review November 23, 2023 14:23
tonistiigi
tonistiigi reviewed Nov 28, 2023
View reviewed changes
Copy link
Member

@tonistiigi tonistiigi left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better output could be linux/arm64, linux/amd64 (N more)

For the amd64 variants, I guess we could just show the maximum variant with truncated mode.

@crazy-max crazy-max force-pushed the ls-notrunc branch 2 times, most recently from cda6a69 to 3a19fa5 Compare November 30, 2023 15:12
@crazy-max
Copy link
Member Author

I think a better output could be linux/arm64, linux/amd64 (N more)

👍

For the amd64 variants, I guess we could just show the maximum variant with truncated mode.

Hum so for linux/amd64, linux/amd64/v2, linux/amd64/v3 we should only display linux/amd64/v3 or linux/amd64, linux/amd64/v3? User might be confused if we just display max variant only. I guess this is similar to linux/arm and linux/arm/v7 🤔

@tonistiigi
Copy link
Member

User might be confused if we just display max variant only.

If you think it is confusing, then I think better to not show variant at all in truncated view. We should not show linux/amd64/v1 or linux/amd64/v2 if that is not the max variant. The question is though if linux/amd64 (2 more) untruncated to linux/amd64 linux/amd64/v2 linux/amd64/v3` makes much sense.

crazy-max
crazy-max commented Dec 12, 2023
View reviewed changes
commands/ls_test.go Outdated
Comment on lines 18 to 19
platforms: []string{"linux/arm64*", "linux/amd64", "linux/amd64/v2", "linux/riscv64", "linux/ppc64le", "linux/s390x", "linux/386", "linux/mips64le", "linux/mips64", "linux/arm/v7", "linux/arm/v6"},
expected: []string{"linux/arm64*", "linux/amd64", "linux/arm/v7", "linux/ppc64le"},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the priority over popular platforms to iterate over them until finding a match instead of using a map so we add some weight from the list.

I think this truncated output makes sense based on popular CPU architectures. I will take a look about stripping the variant.

@crazy-max crazy-max marked this pull request as draft December 23, 2023 16:37
@thompson-shaun thompson-shaun added this to the v0.19.0 milestone Oct 2, 2024
@crazy-max
ls: no-trunc opt
820f5e7 
Signed-off-by: CrazyMax <[email protected]>
@crazy-max crazy-max modified the milestones: v0.19.0, v0.18.0 Oct 3, 2024
@crazy-max
Copy link
Member Author

@tonistiigi Updated to take variants into account, see PR description.

@crazy-max crazy-max marked this pull request as ready for review October 3, 2024 09:23
@tonistiigi tonistiigi merged commit 7c91f3d into docker:master Oct 3, 2024
108 checks passed
@crazy-max crazy-max deleted the ls-notrunc branch October 3, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

Assignees

@tonistiigi tonistiigi

Labels
area/cli area/docs area/tests
Projects
None yet
Milestone
v0.18.0
Development

Successfully merging this pull request may close these issues.
3 participants
@crazy-max @tonistiigi @thompson-shaun